Skip to content

Braidingtensor planarcontract! fixes#418

Merged
lkdvos merged 13 commits intomainfrom
ld-fixes
Apr 30, 2026
Merged

Braidingtensor planarcontract! fixes#418
lkdvos merged 13 commits intomainfrom
ld-fixes

Conversation

@lkdvos
Copy link
Copy Markdown
Member

@lkdvos lkdvos commented Apr 26, 2026

This was overlooked in the release, and unfortunately now it is a bit annoying to backport this to 0.16 (although we may discuss if this could be worth it, as a true bugfix).

To do:

  • Add tests that actually cover this code.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 26, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/tensors/braidingtensor.jl 80.41% <100.00%> (+10.02%) ⬆️

... and 23 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lkdvos lkdvos force-pushed the ld-fixes branch 4 times, most recently from 2d4f673 to b73f35e Compare April 28, 2026 19:51
@lkdvos lkdvos marked this pull request as ready for review April 28, 2026 19:51
@lkdvos lkdvos requested review from Jutho and kshyatt April 28, 2026 19:53
Comment thread src/tensors/braidingtensor.jl Outdated
Comment thread src/tensors/braidingtensor.jl Outdated
Jutho
Jutho previously approved these changes Apr 29, 2026
@lkdvos
Copy link
Copy Markdown
Member Author

lkdvos commented Apr 30, 2026

So, it turns out that I made a mistake in simply replacing the entire call with add_braid!, since that is not actually equivalent to transpose + artin_braid, which is what the BraidingTensor is supposed to do.
This caused me a lot of painful debugging, but I think I now figured it out :).

I know that we could still consider merging this by writing a new fusiontree manipulation that does this, but unless this is really showing up in the profiler I would argue that it is fine to leave this as is for now.

@lkdvos
Copy link
Copy Markdown
Member Author

lkdvos commented Apr 30, 2026

Failures are unrelated (still the MatrixAlgebraKit issues), so will merge this as-is.

@lkdvos lkdvos merged commit 5861432 into main Apr 30, 2026
32 of 40 checks passed
@lkdvos lkdvos deleted the ld-fixes branch April 30, 2026 20:02
lkdvos added a commit that referenced this pull request Apr 30, 2026
* update BraidingTensor `planarcontract!`

* remove obsolete code

* add `promote_storagetype` edge case

* add braidingtensor.jl tests

* extend braidingtensor tests

* update planarcontract implementation

* fix small typos

* one more bugfix

* try once more

* some more tests

* Correct braiding behavior

* remove unnecessary overloads

* add_permute does not accept allocators
@lkdvos lkdvos mentioned this pull request Apr 30, 2026
@Jutho
Copy link
Copy Markdown
Member

Jutho commented Apr 30, 2026

So, it turns out that I made a mistake in simply replacing the entire call with add_braid!, since that is not actually equivalent to transpose + artin_braid, which is what the BraidingTensor is supposed to do. This caused me a lot of painful debugging, but I think I now figured it out :).

I should have known this. I had some hunch while reviewing that something might be off, and was looking for the effect of the final pAB permutation, but then I stopped worrying about it and assumed that it would be fine, with the permutation really only doing the swap and therefore add_braid! being the right primitive. But indeed, it is not.

@lkdvos
Copy link
Copy Markdown
Member Author

lkdvos commented Apr 30, 2026

Yeah, it is very confusing since the @planar macro actually only generates calls where the pAB turns out to be trivial, which is why I somehow assumed it would be fine.
I at least put in some comments to prevent me (and anyone else in the future) from making the same mistake again :)

lkdvos referenced this pull request May 1, 2026
* Update changelog for v0.16.5

* Bump version to v0.16.5

* Update CITATION.cff for v0.16.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants